-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[util-linux-libuuid] Add util-linux-libuuid package #17664
[util-linux-libuuid] Add util-linux-libuuid package #17664
Conversation
* This is the most actively maintained fork of libuuid * This package should supersede the existing libuuid recipe, which should be deprecated as an inactive fork of the original libuuid. Closes conan-io#17337
This comment has been minimized.
This comment has been minimized.
5a8cf99
to
bea16f8
Compare
This comment has been minimized.
This comment has been minimized.
… compiler or build_type don't exist
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…libintl * libintl ships as part of glibc on linux and so this isn't required in that context. On Macos, this isn't the case so this has to be linked against explicitly
This comment has been minimized.
This comment has been minimized.
I haven't been able to replicate the error message on my mac locally:
If anybody has any suggestions on how to replicate this it would be appreciated. |
This comment has been minimized.
This comment has been minimized.
* Invalidate Macos builds as they are failing in a way that hasn't been able to be reproduced for debugging on the CI machine.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
366e19b
to
26b528d
Compare
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 14 (
Conan v2 pipeline ✔️
All green in build 14 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @samuel-emrys!
@RubenRBS @memsharded @czoido @uilianries can one of you review this PR ? thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@samuel-emrys Thanks for adding this! |
to avoid conflict with system libuuid see conan-io#17664 and conan-io#17427 (comment) for motivation and rationale
self.cpp_info.set_property("cmake_target_name", "LibUUID::LibUUID") | ||
self.cpp_info.set_property("cmake_file_name", "LibUUID") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does it come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #17664 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an internal file of CMake source code, not some official FindLIBUUID.cmake file shipped in CMake program, so it shouldn't be used as a reference. This recipe shouldn't override cmake names since there is no official Find file (someone asked for it, but it will likely be rejected by CMake team: https://gitlab.kitware.com/cmake/cmake/-/issues/24607), and util-linux project doesn't provide any config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I'd missed that detail. Knowing that a convention has been used by CMake though (even though it's internal), wouldn't that still be the sensible choice for Conan? Since util-linux provides no config file, I would imagine people will take what already exists. Seems like this might be a sensible convention to standardise on..
self.tool_requires("libtool/2.4.7") | ||
self.tool_requires("m4/1.4.19") | ||
self.tool_requires("pkgconf/1.9.3") | ||
self.tool_requires("bison/3.8.2") | ||
self.tool_requires("autoconf/2.71") | ||
self.tool_requires("automake/1.16.5") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very bad idea to explicitly reference automake, autoconf and m4, it doesn't scale in build requirements, it's well known. Why this recipe doesn't follow other autoconf recipes guidelines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not experienced with automake. I used a few other recipes as guidelines but they obviously haven't incorporated the practices you're talking about and it didn't come up in review. It would be useful if those were documented somewhere easily discoverable. Feel free to submit a new PR with the necessary changes if that's a better way of managing these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good reference is autotools recipes template.
(sorry, I won't fix this, I don't have the time. It will break when someone will bump automake version in libtool recipe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know, but it wouldn't have addressed this - it has an explicit example requiring automake, and no discussion of when it shouldn't be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it has an explicit example requiring automake, and no discussion of when it shouldn't be required.
There is a comment explaining when it's required: https://github.com/conan-io/conan-center-index/blob/60fa77f44eafacf99b7f1d43650f13dd8db5d971/docs/package_templates/autotools_package/all/conanfile.py#L93C1-L96
* [util-linux-libuuid] Add util-linux-libuuid package * This is the most actively maintained fork of libuuid * This package should supersede the existing libuuid recipe, which should be deprecated as an inactive fork of the original libuuid. Closes conan-io#17337 * [util-linux-libuuid] Add minimum compatible compiler versions * [util-linux-libuuid] Add apple-clang to minimum compiler list * [util-linux-libuuid] Add failover for min_version call to return 0 if compiler or build_type don't exist * [util-linux-libuuid] Add dependency on gettext for Macos to bring in libintl * libintl ships as part of glibc on linux and so this isn't required in that context. On Macos, this isn't the case so this has to be linked against explicitly * [util-linux-libuuid] Use libgettext instead of gettext for libintl * [util-linux-libuuid] Invalidate macos builds * Invalidate Macos builds as they are failing in a way that hasn't been able to be reproduced for debugging on the CI machine. * [util-linux-libuuid] Modify version to use util-linux version rather than libuuid * Update CMake targets to match upstream libuuid -> LibUUID Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com> * [util-linux-libuuid] Fix consumer find_package to match new target name --------- Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>
system
package is used withlibuuid
in the dependency tree, such asxorg/system
. See [tk] Migrate tk to conan 2.0 #17427 (comment) for a concrete example of this.Closes #17337
Specify library name and version: libuuid/2.39